Skip to content

Centralize the equation-of-state expressions in the Riemann solvers - #1762

Open
sbryngelson wants to merge 62 commits into
MFlowCode:masterfrom
sbryngelson:fix/speed-of-sound-from-state
Open

Centralize the equation-of-state expressions in the Riemann solvers#1762
sbryngelson wants to merge 62 commits into
MFlowCode:masterfrom
sbryngelson:fix/speed-of-sound-from-state

Conversation

@sbryngelson

@sbryngelson sbryngelson commented Aug 25, 2026

Copy link
Copy Markdown
Member

Gives each equation-of-state expression one definition, so that adding a second EOS (#1638) is a
change inside one module rather than an edit to every solver.

The operators

s_compute_mixture_coefficients(alpha_rho, alpha, rho, gamma, pi_inf, qv)
s_compute_energy(pres, alpha_rho, alpha, vel_sum, E)
s_compute_speed_of_sound(pres, rho, gamma, pi_inf, adv, c)   ! _avg for interface averages
f_bulk_modulus(pres, gamma, pi_inf)
f_pressure(e_int, gamma, pi_inf, qv)
f_phase_internal_energy(pres, alpha, alpha_rho, gamma, pi_inf, qv)
f_isentrope_exponent(gamma) / f_isentrope_pressure(pi_inf, gamma)

s_compute_speed_of_sound used to take H, |u|^2 and qv and then undo them; for a real state all
three cancel, so it takes none of them and #1707 becomes unrepresentable.

The f_ operators take coefficients rather than a fluid index, so a mixture - whose effective gamma
and pi_inf already come from s_compute_mixture_coefficients - is the same call as a single fluid.
That is what lets the sound speed use the bulk modulus: every one of its cases is a bulk modulus over a
density, differing only in how the phases mix (Wood's law is their harmonic mean, the six-equation model
their arithmetic mean, the rest the mixture value). Written that way the mpp_lim sub-branch of the
bubbles case is visibly the same expression as the plain case and collapses. The four rewrites were
checked against the originals over 200k random states, agreeing to three ulp.

gs_min and ps_inf already are the Tait exponent and reference pressure, 1/gamma + 1 and
pi_inf/(1 + gamma). Three places rebuilt them by hand - m_qbmm's pi_infs(1)*(n - 1)/n reduces
exactly to ps_inf(1) - and HLLC's star state now reads as the relation it is, p* = (p + B) xi**n - B.

Kinetic, magnetic and elastic energy stay at the call sites; they are not EOS terms.
s_compute_pressure decides what the internal energy is, then inverts once.

Copies collapsed: mixture rule 9, bulk modulus 7, pressure inversion 5, elastic strain energy 4,
per-phase internal energy 3, isentrope parameters 3.

A second sweep for inline stiffened-gas arithmetic found four more, all now routed through the
operators: post_process's three derived quantities (specific heat ratio, liquid stiffness, sound
speed - the last sat beside a call already using f_bulk_modulus); s_compute_cson_from_pinf, a
fifth sound-speed copy in the E/H round-trip form #1707 was about, which built E and H only
to cancel |u|^2 - a real cancellation loss, not just verbosity, and its deletion also removes a
USING_AMD workaround and two arguments from a [seq] device routine; and the pressure-based wave
speed in HLL and HLLC, whose (0.5 + Gamma)/(1 + Gamma) is exactly Toro's two-shock coefficient
(n + 1)/2n and whose (p*/p - 1)*p divided by p only to multiply it back.

Behaviour changes

Each is a forward/inverse disagreement or a wrong mixture, none exercised by any test:

  • hll and lf lacked the bubbles_euler special case, computing gamma = alf*gammas(1) with
    alf a void fraction. They now take HLLC's rule, which is the published one: under bubbles_euler
    the last advection slot is the void, not a material (eqn_idx%alf == eqn_idx%adv%end), so the
    mixture sums the material slots only and a single carrier liquid keeps its own coefficients
    undiluted - Gamma_l p_l = (E - rho|u|^2/2)/(1 - alpha) - Pi_inf_l, the void entering only through
    the 1/(1 - alpha) that s_compute_pressure already applies. HLLC and m_acoustic_src are
    unchanged by this; a two-fluid bubbly case is added, since nothing ran bubbles_euler above one
    fluid. num_fluids > 2 is now refused at validation: the closure is written for one carrier liquid, the
    two arms above it disagreed, and nothing exercised either (bubbles_euler with num_fluids > 2 runs an unvalidated mixture closure #1786).
  • m_cbc had the same gap in derivative form. It differentiates the mixture rule with no
    bubbles_euler case, so three of its four coefficient derivatives were wrong there - they should
    vanish, because the rule makes gamma, pi_inf and qv constants.
  • HLLC's bubbles energy dropped qv while s_compute_pressure subtracts it.
  • m_ibm dropped qv twice (ghost-point energy and per-phase internal energy), put the dynamic
    pressure inside the (1 - alpha) scaling instead of outside, and used alpha_IP(1) where
    eqn_idx%alf is alpha_IP(num_fluids).
  • post_process's energy budget formed sum(alpha*alpha*rho) rather than sum(alpha*rho); that
    density feeds the sound speed, so its Mach number was wrong wherever a volume fraction was below one.
    Its gas internal energy Egint was alpha_g*gamma_g*p, missing the alpha_g*pi_inf_g + alpha_rho_g*qv_g
    that make up the rest of a stiffened-gas internal energy; it now calls f_phase_internal_energy, so it
    agrees with the pressure inversion that produced p. Identical for a gas phase with pi_inf = qv = 0.
  • The hypoelastic probe pressure rebuilt the kinetic energy from one momentum component while the
    correct dyn_p was passed in and ignored - wrong in 2D and 3D. mom is gone from
    s_compute_pressure, so no caller can reintroduce it.
  • m_acoustic_src never applied the subgrid void correction. It was the last site hand-rolling a
    sound speed. The Tait form it used is the same stiffened-gas expression as f_bulk_modulus - with
    n = 1/Gamma + 1, (n-1)/n = 1/(1+Gamma) - so that half is pure de-duplication, and the downstream
    c/(small_gamma - 1) was c*Gamma written the long way round. The behaviour change is the dilution:
    a wave injected into a bubbly medium travels at the mixture speed c_l/(1 - alf), and the hand-rolled
    code used the undiluted carrier-liquid speed. 39 goldens move, all bubbles_euler, all by ~1.7% at
    the source cell - that is 1/(1 - alf) at alf = 0.04, and it is visible only because the test grids
    are deliberately coarse (49x39 in 2D against 299 in 1D), so a 2% shift at the source sits well outside a
    1e-10 tolerance. Lagrange and non-bubbly acoustic cases do not move: the guard excludes them.
    The correction is inlined rather than calling s_compute_speed_of_sound, which faults CCE OpenACC from
    that loop for reasons not yet isolated (CCE OpenACC faults when the acoustic loop calls s_compute_speed_of_sound #1794); the comment there says to fold it back once they are.
  • The subgrid void fraction is now named where it is used (Bubble sound speed: alpha -> 1 should assert, not silently produce Inf #1793). The bubble branch of the mixture
    sound speed is c = c_l/(1 - alf) - the carrier-liquid speed with an O(alf) correction, the same c_l
    that m_qbmm forms for Keller-Miksis. alf is dilute by construction, so a value near one is a wrong
    index or a case outside the expansion's regime, not a number to clamp; the toolchain warns at case load
    instead. No arithmetic changes.
  • relativity silently ignored pi_inf. The relativistic enthalpy h = 1 + (Gamma + 1)p/rho
    carries no stiffness, and it was written out twice - once in m_riemann_solver_hll under a
    ! Hard-coded EOS comment, once inside s_compute_speed_of_sound's own relativity branch. It is
    now f_relativistic_enthalpy, and a stiffened fluid with relativity is refused at validation
    rather than quietly given an ideal-gas answer.

Test comparison

A golden that records an analytically zero field stores only roundoff, and comparing it pointwise tests
the compiler's association order: 421A6AD9 failed on a sign flip of a 5.8e-07 crumb. A field whose
every golden value already lies within the absolute tolerance of zero is now checked for staying in a
band rather than for reproducing its residue. This also closes a hole - compute_error returns NaN
when the golden value is exactly 0 and is_close passed NaN unconditionally, so ~2800 fields were
not checked at all.

The band is the larger of the tolerance and ten times the golden's own magnitude, not the tolerance
alone. Held to the bare tolerance the rule misfires on a field that merely sits near it: single
precision scales QBMM's 1e-10 by 1e8, so its tolerance is 1e-2 and its bubble fields are
constants at 0.9999997 of that - read as zero, then failed for drifting a few ulps past it. Scaling
by the golden judges such a field against itself. A field storing genuine roundoff is orders below the
tolerance, so its band is unchanged: the 5.8e-07 sign flip still passes and a candidate growing from
roundoff to 1e-3 still fails.

Verification

MI210, AFAR amdflang, OpenMP offload. 5eq_rk3_weno3_hllc, three runs each, one node per comparison:

mean ns/gp/eq/rhs
before 1.4742
after 1.4738

0 regression(s) across 471 kernels in static GPU resources, so the [seq] operators inline even in
the per-phase, per-face HLLC flux loop.

Full suite 684 passed. Targeted set of 196 covering every riemann_solver, avg_state,
wave_speeds, low_Mach and mixture_err combination plus chemistry, reactive burn, phase change
and hypoelasticity - 196 passed, 0 failed. The only golden regenerated is the new 2D probe case,
for #1773 below.

The nvfortran fix was verified as a before/after pair on one machine, nvfortran 24.1 in a container:
the pre-fix tree reproduces fort2 TERMINATED by signal 11 on the HLL solver and the post-fix tree
builds clean under both --gpu mp and --gpu acc. Reverting only the molecular_weights change
brings the crash back, which is what attributes it. The two CCE fixes were confirmed on Frontier -
the device-global rule by a six-build bisection, and the runtime fault by all four CCE lanes passing
once the thermochem calls moved to the loop body.

Scope

Deliberately left. Each says what it would take, so none of it needs rediscovering:

Supersedes #1714 (same goal; its type(eos_state) cost 20% because a derived-type dummy on a [seq]
device routine forces a memory ABI). Closes #1707, #1708, #1715, #1769, #1773, #1774, #1777, #1778, #1781, #1786. Unblocks #1638.

Also filed from this work, not fixed here: #1784, #1785, #1793, #1794, #1779 (fixed upstream by #1780),
and the audit comments on #1682, #1687 and #1693 correcting their stated impact.

m_acoustic_src was the last site hand-rolling a sound speed; it now uses f_bulk_modulus and applies
the subgrid void correction. See the behaviour-change entry above. The remaining open item is the CCE
mechanism (#1794), not the physics.

Bisect caveat: a94f914a is not independently runnable. Its case_validator.py calls
check_eos_selector, whose CONSTRAINTS["fluid_pp(1)%eos"] entry does not arrive until f009397c,
so ./mfc.sh at exactly that commit raises KeyError. The commits were split by file rather than by
hunk. The branch tip is correct; only a bisect landing on that one commit sees it.

Folded in: the Riemann macro cleanup (#1769)

The inline_riemann.fpp / inline_capillary.fpp macros inline computational statements into the
solver loops and bind to the caller's locals by name. A missing local is reported at the call site
rather than in the macro, the bodies do not appear when reading the solver, and every variable they
touch has to be carried by hand in the GPU private() lists.

All nine are now procedures, and both include files are deleted:

macro replaced by sites
hll_flux_component f_hll_flux 3
compute_capillary_stress_tensor s_compute_capillary_stress_tensor 3
compute_axis_inv_re s_compute_axis_inv_re 4
compute_elastic_wave_speeds_lr f_elastic_signal_speed 2
compute_hypo_elastic_energy f_elastic_energy 1
compute_low_Mach_correction f_low_Mach_zcoef, f_low_Mach_pcorr_hll, f_low_Mach_pcorr_hllc, s_apply_low_Mach_velocity 8
compute_average_state, roe_avg, arithmetic_avg s_compute_average_state, s_compute_chemistry_average_state 3

inline_riemann.fpp and inline_capillary.fpp are both gone. The Lax-Friedrichs solver expanded
compute_average_state and used none of it, so that call site is deleted rather than converted -
which is what #1715 was reporting.

The low-Mach one is the interesting case. It branched on riemann_solver and on low_Mach at
runtime, but every one of its eight call sites already knew both answers: low_Mach = 2 requires
riemann_solver = 2, HLLD forbids low_Mach entirely, and each site sits inside its own low_Mach
guard in a file that is one solver. Every branch in the macro was dead at every site, so it was split
by leaf. The two pcorr functions absorb the low_Mach == 1 test and return zero otherwise, which
drops the repeated else pcorr = 0 from five sites; the low_Mach == 2 guard stays visible at its
three sites because that leaf mutates the wave-normal velocities before the wave speeds are
computed. zcoef, vel_L_tmp and vel_R_tmp become local and leave five GPU private lists.

Static GPU resources: 0 regression(s) across 471 kernels for the low-Mach change, measured as a
before/after pair on one node.

Compiler portability

Extracting solver code into $:GPU_ROUTINE device routines is not compiler-neutral. Two constructs
built cleanly under amdflang OpenMP offload and CCE OpenMP, and broke elsewhere:

CCE OpenACC rejected the pressure-relaxation solver:

ftn-7066 ftn: ERROR S_CORRECT_INTERNAL_ENERGIES, m_pressure_relaxation.fpp, Line = 211
  Unsupported accelerator code error: Global in accelerator routine without declare -- num_fluids

Six builds on Frontier pinned the rule down. An array whose bound is a device global may be passed
to a device routine from a parallel-loop body, but not from inside another acc routine seq.
Three plausible readings are wrong, each measured rather than argued: it is not call depth (moving
the construct up one level only moved the error), not the optional dummies (an operator with none
still failed), and not the local's bound (declaring it dimension(num_fluids_max), a parameter,
still failed). A variant keeping the num_fluids locals, the gather loop and every in-module call
built once the one cross-routine array pass was removed.

The reported line is where the compiler gave up, not the cause: remove one trigger and the message
walks forward to the next call. And only the plain lanes fail - under --case-optimization those
bounds are parameters, so the green Case Opt lane beside a failing plain one is the signature.

Every accepted call site in the tree already obeys the rule - m_cbc, m_ibm, m_bubbles_EL and
s_compute_cell_state all call this kernel from a loop body - and only this file did not. So the
loop body absorbs s_relax_cell_pressure, which existed only to hold three per-cell calls, and the
mixture goes back through the shared operator. The file is 38 lines shorter for it.

nvfortran 23.11 and 24.1 segfaulted in fort2 compiling the HLL solver, chemistry config only.
s_compute_chemistry_average_state took molecular_weights - a parameter array from
m_thermochem - as an actual argument into a declare-target routine. Every other use in MFC reads
that array directly in the kernel. It now takes the per-species gas constants, formed at the call
site. Confirmed as an A/B/A on one machine: reverting only this change brings the ICE back, so it is
this and not the accompanying dead-local removal.

CCE OpenMP and OpenACC both faulted the GPU at runtime on the two reacting-Roe chemistry cases -
Memory access fault by GPU node-N ... Reason: Unknown on the first step, with a clean build.
s_compute_chemistry_average_state called m_thermochem one routine deeper than master did, holding
seven num_species automatic arrays in the intermediate frame. This is the same rule as the
num_fluids arrays above, in a second guise: such calls belong in the parallel-loop body. The
enthalpies and heat capacities are now evaluated at the call sites and passed in, leaving the routine
pure arithmetic over three arrays - and both call sites already computed the heat capacities, so it
had been recomputing them. Chem-config GPU resources move by two registers on one kernel of 473.

Note that cray_inline / cray_noinline / function_name on GPU_ROUTINE are no-ops on the
OpenACC path - all three branches emit the same !$acc routine, and the !DIR$ hints appear only on
Cray CPU builds. Neither of these is fixable by an inlining hint.

Fix: the Roe average dropped the transverse kinetic energy

Found while centralizing the sound speed. roe_avg accumulated vel_avg_rms over all velocity
components and then overwrote it with the first component alone:

do i = 1, num_vels
    vel_avg_rms = vel_avg_rms + (sqrt(rho_L)*vel_L(i) + sqrt(rho_R)*vel_R(i))**2._wp/(sqrt(rho_L) + sqrt(rho_R))**2._wp
end do
...
vel_avg_rms = (sqrt(rho_L)*vel_L(1) + sqrt(rho_R)*vel_R(1))**2._wp/(sqrt(rho_L) + sqrt(rho_R))**2._wp

vel_avg_rms has exactly one consumer, s_compute_speed_of_sound_avg, which needs the full squared
magnitude of the averaged velocity. Four things agree the loop is right and the overwrite is wrong:

  1. c = (H - 0.5*vel_sum - qv/rho)/gamma is the stiffened-gas Roe sound speed, in which vel_sum is
    |u|^2 summed over all components.
  2. It is not even the normal component. dir_idx is (1,2,3), (2,1,3), (3,1,2) per sweep and
    momentum stays in physical order, so vel_L(dir_idx(1)) is the normal velocity and vel_L(1) is
    the x-component in every sweep - a transverse velocity in the y and z sweeps.
  3. arithmetic_avg keeps the full sum, and so does the hand-written bubbles path at
    m_riemann_solver_hllc.fpp:625. Only roe_avg overwrote.
  4. In the chemistry branch vel_avg_rms appears twice with opposite sign - once inside Phi_avg,
    once as vel_sum - and the two cancel to leave c^2 = gamma*R*T exactly, but only for the full
    magnitude. With the overwrite the residue is + (gamma-1)*|u_transverse|^2/2.

Deleting the overwrite line is the fix. The error is zero in 1D (num_vels == 1), which is why the
two flamelet examples that use avg_state = 1 with wave_speeds = 2 never showed it.

Why it survived: roe_avg runs only when avg_state == 1 and wave_speeds == 2, and
cases.py pushed those as sibling cases off the same stack level, never combined - so the
avg_state=1 cases computed the average and discarded it, and the wave_speeds=2 cases took the
arithmetic branch. The path had no effective coverage. This PR adds the missing combination for 1D,
2D and 3D against HLL and HLLC.

Generating those six goldens on the unfixed code and then regenerating after the fix gives exactly
the predicted split: the two 1D goldens come back byte-identical, all four 2D/3D goldens move.
No pre-existing golden changes - roe_avg is reachable only through compute_average_state, which
is called only under wave_speeds == wave_speeds_pressure, so {avg_state == 1 and wave_speeds == 2} is the complete affected set and nothing else in the tree is in it.

Verification: 97 targeted cases covering every riemann_solver, avg_state, wave_speeds,
low_Mach and mixture_err combination across 1D/2D/3D and both fluid counts - 97 passed, 0
failed. Static GPU resources 0 regression(s) across 471 kernels for the Roe fix and again for the
low-Mach conversion, each measured as a before/after pair on one node.

Fix: the hypoelastic strain energy summed one stress component for all of them (#1773)

s_compute_pressure declared stress as a scalar and then added it once per stress index, so the
elastic energy was 4*tau_11^2 instead of tau_11^2 + 2*tau_12^2 + tau_22^2. Every caller passes
eqn_idx%stress%beg. Correct in 1D, where the one component is not a shear one; wrong in 2D and 3D.

The caller now supplies the summed energy, which removes the loop, shear_indices and a loop
iterator from a device routine in src/common. f_elastic_energy already computed the per-component
term, so it moves from m_riemann_state to m_variables_conversion beside the other operators and
gains callers rather than copies. The conservative-to-primitive conversion carried a fourth copy in
the per-cell GPU path; it now calls the same function, which also absorbs its near-zero shear-modulus
guard and its shear doubling - ten lines become three. Four copies become one, gated on that hot path
by unchanged goldens and 0 regression(s) across 471 kernels.

Only probe output reaches this branch, and no test enabled probe_wrt on a multi-dimensional
hypoelastic case, so the fix brings one: 2D, one fluid, tau_e = (-1e4, 3e3, -2e3). Equal components
would hide the bug - both forms give 4*tau^2 - so the stresses have to differ.

elastic energy probe pressure
before 1000.0 97683.33333333
after 305.0 100000.0

The corrected value is the pressure the case specifies, a 2.4% error removed, and it matches a
hand-evaluation of (E_buggy - E_correct)/gamma to every printed digit. No other field in that
golden moves, and the three 1D hypoelastic cases are unchanged, as the 1D-is-correct argument
predicts.

Fix: HLLC never used the reacting Roe sound speed (#1774)

Two halves. HLL read c_sum_Yi_Phi uninitialized on the arithmetic path - .and. is not required to
short-circuit, so the read happens even though the value is unused. It is now zeroed before the
branch.

The other half is a wrong answer rather than a latent one. The HLLC base path formed the chemistry
average state and then passed a literal 0._wp; since abs(c_c) > verysmall gates the branch, it
always fell back to the frozen sound speed, while HLL and the hypoelastic variant used the Roe one.
The same case got a different sound speed depending on which solver ran it and whether
hypoelasticity was on.

The two are not interchangeable. c_c - (gamma - 1)*(vel_sum - H) reduces exactly to the Roe
sound speed (gamma - 1)*(H_avg - |u_avg|^2/2) - the (gamma - 1)*h_avg and gamma*R*T_avg terms
cancel - whereas the fallback is the frozen mixture speed evaluated at pres_R with rho_avg (which
the original comment called placeholders) and drops the velocity-variance term the linearization
needs. On a representative shocktube state they differ by ~2%. avg_state = 1 asks for Roe
averaging, so the Roe sound speed is what it should get.

The bubbles_euler loop keeps its literal zero and now says why: it never forms the chemistry
average, and chemistry with bubbles_euler/qbmm is prohibited, so that branch is unreachable.
Replacing that zero too would have reintroduced the uninitialized read.

Of the fourteen chemistry cases, only the HLL case added here reached the Roe branch at all, so the
HLLC combination had no coverage and no existing golden moves. A riemann_solver = 2 case is added
beside it.

s_compute_speed_of_sound took H, |u|^2 and qv and then undid them: for a real state H = ((Gamma+1)p + Pi + qv)/rho + |u|^2/2, so c^2 = (H - |u|^2/2 - qv/rho)/Gamma reduces to ((Gamma+1)p + Pi)/(Gamma rho). The three arguments cancel.

Callers therefore no longer supply them, and the invisible 'H must include qv' contract cannot be stated. That contract is what produced MFlowCode#1707: five sites open-coded H and three dropped the qv this routine went on to subtract. Those three are fixed here by construction.

An average of two states is not a state - its enthalpy is a free input - so the four interface-averaged sites use s_compute_speed_of_sound_avg, whose arithmetic is unchanged.

Falls out as dead: qv_sf (a full-domain array whose only reader was one of the open-coded enthalpies), the H dummy threaded through s_save_data and p_main, and a per-cell qv accumulation in post-process.

Claude-Session: https://claude.ai/code/session_011BUQDw64EtzvDzTvtWTNj4
@github-actions

Copy link
Copy Markdown

Claude Code Review

Head SHA: 9974b68

Files changed:

  • 12
  • src/common/m_variables_conversion.fpp
  • src/post_process/m_data_output.fpp
  • src/post_process/m_start_up.fpp
  • src/post_process/p_main.fpp
  • src/simulation/m_cbc.fpp
  • src/simulation/m_data_output.fpp
  • src/simulation/m_riemann_solver_hll.fpp
  • src/simulation/m_riemann_solver_hllc.fpp
  • src/simulation/m_riemann_solver_hlld.fpp
  • src/simulation/m_riemann_solver_hypo_hlld.fpp
  • src/simulation/m_riemann_solver_lf.fpp
  • src/simulation/m_time_steppers.fpp

Findings:

  • src/simulation/m_riemann_solver_hypo_hlld.fpp: the two s_compute_speed_of_sound calls near the diff (previously passing H%L/H%R) are now the only reason H%L = (E%L + pres%L)/rho%L and H%R = (E%R + pres%R)/rho%R (a few lines above, outside this hunk) were computed — with those arguments dropped, H%L/H%R are written but never read anywhere else in the file, and the H component stays a dead entry in the routine's GPU_PARALLEL_LOOP private-variable list. Every other file touched by this same refactor (hll.fpp, hlld.fpp, cbc.fpp) had its now-unused H computation removed or kept alive by a real remaining use; this one file was missed, leaving dead computation the PR's own stated goal ("delete rather than open-code") argues against.

@sbryngelson
sbryngelson marked this pull request as ready for review August 25, 2026 15:56
@sbryngelson
sbryngelson requested a lite review from Copilot and removed request for Copilot August 25, 2026 15:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Derives real-state sound speed directly from thermodynamic state while preserving enthalpy-based calculations for interface averages.

Changes:

  • Simplifies real-state sound-speed calls across simulation and post-processing.
  • Adds a dedicated interface-average sound-speed routine.
  • Removes obsolete enthalpy threading and qv_sf storage.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/common/m_variables_conversion.fpp Implements split sound-speed APIs and removes qv_sf.
src/simulation/m_time_steppers.fpp Uses state-derived sound speed.
src/simulation/m_riemann_solver_lf.fpp Simplifies LF sound-speed calls.
src/simulation/m_riemann_solver_hypo_hlld.fpp Simplifies hypoelastic HLLD calls.
src/simulation/m_riemann_solver_hlld.fpp Simplifies HLLD calls.
src/simulation/m_riemann_solver_hllc.fpp Separates real and averaged-state calculations.
src/simulation/m_riemann_solver_hll.fpp Separates real and averaged-state calculations.
src/simulation/m_data_output.fpp Corrects diagnostic sound-speed computation.
src/simulation/m_cbc.fpp Removes unnecessary enthalpy calculation.
src/post_process/p_main.fpp Removes obsolete enthalpy argument.
src/post_process/m_start_up.fpp Simplifies sound-speed output generation.
src/post_process/m_data_output.fpp Removes redundant qv and enthalpy accumulation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

! Compute mixture sound Speed
call s_compute_speed_of_sound(pres, rho, gamma, pi_inf, ((gamma + 1._wp)*pres + pi_inf)/rho, alpha, 0._wp, &
& 0._wp, c, qv)
call s_compute_speed_of_sound(pres, rho, gamma, pi_inf, alpha, c)
@codecov

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.95824% with 95 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.24%. Comparing base (41df185) to head (e07ee64).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
src/simulation/m_viscous.fpp 33.33% 20 Missing and 10 partials ⚠️
src/common/m_variables_conversion.fpp 85.96% 8 Missing and 8 partials ⚠️
src/simulation/m_riemann_solver_hllc.fpp 68.42% 11 Missing and 1 partial ⚠️
src/simulation/m_riemann_solver_hlld.fpp 0.00% 8 Missing ⚠️
src/post_process/m_data_output.fpp 0.00% 6 Missing ⚠️
src/simulation/m_pressure_relaxation.fpp 72.22% 1 Missing and 4 partials ⚠️
src/simulation/m_riemann_state.fpp 92.00% 0 Missing and 4 partials ⚠️
src/post_process/m_derived_variables.fpp 40.00% 3 Missing ⚠️
src/simulation/m_data_output.fpp 62.50% 3 Missing ⚠️
src/pre_process/m_assign_variables.fpp 0.00% 2 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1762      +/-   ##
==========================================
+ Coverage   61.67%   62.24%   +0.56%     
==========================================
  Files          84       84              
  Lines       21619    21552      -67     
  Branches     3196     3187       -9     
==========================================
+ Hits        13334    13414      +80     
+ Misses       6093     5940     -153     
- Partials     2192     2198       +6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

s_accumulate_mixture_properties and the else branch of s_convert_species_to_mixture_variables_kernel contained the same four-line accumulation loop. Move the routine from m_riemann_state.fpp into m_variables_conversion.fpp and have the kernel call it, below the mpp_lim clipping so it still sees the clipped volume fractions.

The two routines are not merged. s_accumulate_mixture_properties is a parameterised subset accumulator - callers pass num_fluids or num_fluids - 1, and raw or limited volume fractions - while the kernel always spans num_fluids, clips in place, special-cases num_fluids == 1 with bubbles_euler, and optionally emits Re_K and G_K. Merging would need a clipping flag and optional dummies on a [seq] device routine, which is not portable across the offload backends.

This is the single place per-fluid EOS dispatch will enter the stiffened-gas mixture path.

Claude-Session: https://claude.ai/code/session_011BUQDw64EtzvDzTvtWTNj4
Adds s_compute_energy to m_variables_conversion: the stiffened-gas total energy, thermodynamic terms only. Magnetic and elastic energies stay at their call sites because they are not equation-of-state terms, and the chemistry and relativistic forms are left open-coded because they are not this relation.

Converts the ten plain sites in hll, hllc, hypo_hlld and lf. The four MHD sites and the two hllc bubbles sites that omit qv are deliberately left for follow-up commits; the latter changes behaviour.

Also deletes a dead energy assignment in m_cbc: E is read only inside the chemistry branch of the flux update, so the non-chemistry assignment was never used.

Measured on MI210 / amdflang: all 60 Riemann kernel resource profiles identical (scratch, VGPR, AGPR); 5eq_rk3_weno3_hllc 2.290 / 2.385 / 2.407 against a base of 2.374 / 2.422 / 2.361.

Claude-Session: https://claude.ai/code/session_011BUQDw64EtzvDzTvtWTNj4
hll, hllc, hlld, hypo_hlld and lf each derived the stiffened-gas mixture coefficients their own way: two called a shared accumulator, two hand-rolled the loop inline, and hllc carried a three-way branch. They now all call s_compute_mixture_coefficients, which owns the rule including its one special case.

That special case is required, not incidental. Under bubbles_euler with num_fluids == 1 the sole advection slot aliases the void fraction (eqn_idx%alf == eqn_idx%adv%end), so alpha is not a composition there and the mixture rule does not apply; the coefficients are the liquid's. Clipping stays with the callers because it is genuinely not uniform - hll clips local arrays while hllc clips the shared reconstruction buffers in place - and it can never coincide with the special case, since case_validator prohibits mpp_lim with num_fluids == 1.

Behaviour fix, not a refactor: hll and lf previously lacked the special case and computed gamma = alf*gammas(1) under bubbles_euler, which is physically meaningless. No test or example exercises that combination, but it runs today, so results change for anyone using it. hllc bubbles at num_fluids == 2 likewise moves from pure-liquid to standard accumulation; also unexercised.

s_accumulate_mixture_properties is merged in and deleted: after hllc's three-way branch collapsed, its num_fluids - 1 call site was gone and it had exactly one caller. The gpuParallelization.md example that used it is updated.

s_compute_energy now takes composition rather than coefficients, deriving them through the same rule, so no call site holds equation-of-state knowledge. Measured on MI210 / amdflang: hypo_hlld drops 24 VGPR and 24 AGPR because the caller no longer keeps gamma, pi_inf and qv live across the call; hllc and lf gain 8 VGPR at 118-142, far from any occupancy limit. 5eq_rk3_weno3_hllc 2.343 / 2.319 / 2.330 against 2.304 / 2.283 / 2.366 before.

Claude-Session: https://claude.ai/code/session_011BUQDw64EtzvDzTvtWTNj4
@sbryngelson sbryngelson changed the title Derive the speed of sound from the state, not a supplied enthalpy Centralise the equation-of-state expressions so a second EOS can be added in one place Aug 26, 2026
The operator returns the thermodynamic energy; pres_mag is added at the call site, because magnetic energy is not an equation-of-state term and a second EOS backend must not have to know about it.

Also trims the operator doc comments to the facts a reader cannot derive from the code.
The bubbles_euler branch computed E without qv while every other energy site in the file includes it, and s_compute_pressure's bubbles branch subtracts qv when inverting. Forward and inverse transforms disagreed, so E was inconsistent with the pressure it came from whenever fluid_pp(1)%qv is non-zero. E feeds the energy flux directly, so the error was not confined to diagnostics.

Routing the site through s_compute_energy restores qv. No test or example combines bubbles_euler with a non-zero qv, so the suite neither demonstrates the bug nor the fix; the argument is the forward/inverse inconsistency above.

Measured on MI210 / amdflang: HLLC drops 226 to 214 VGPR; 5eq_rk3_weno3_hllc 2.339 / 2.343 / 2.283.
@sbryngelson sbryngelson changed the title Centralise the equation-of-state expressions so a second EOS can be added in one place Centralise the equation-of-state expressions in the Riemann solvers Aug 26, 2026
…e shared rule

m_viscous carried the same three-way branch four times over and m_acoustic_src once more, under Tait naming: B_tait is sum(alpha*pi_infs) and small_gamma is sum(alpha*gammas). Both now call s_compute_mixture_coefficients, leaving one implementation of the mixture rule in the codebase.

m_acoustic_src also had a latent double-accumulation: with bubbles_euler, mpp_lim and num_fluids > 2 both of its blocks ran, adding onto variables the second block does not reset. Unreachable today, since bubbles cases use num_fluids = 1, and gone now.

As with the solvers, hll and lf aside, this gives these paths the bubbles special case they lacked. No test or example combines bubbles_euler with num_fluids > 1, so the num_fluids > 2 arms were unexercised.
@sbryngelson sbryngelson changed the title Centralise the equation-of-state expressions in the Riemann solvers Centralize the equation-of-state expressions in the Riemann solvers Aug 26, 2026
Both computed H = (E + p)/rho and never read it. The sound speed stopped taking an enthalpy earlier in this branch, and neither solver forms an interface average, so nothing consumed them. m_cbc had the same leftover, removed earlier.

hlld keeps its H_no_mag, which genuinely feeds s_compute_fast_magnetosonic_speed.
rho_avg, H_avg, gamma_avg, qv_avg, vel_avg_rms and c_avg are read only by the pressure-based wave-speed estimate, but every solver built them for every face regardless. Under the Roe average that is eight square roots plus an equation-of-state call, discarded whenever wave_speeds is direct - which is the default and every benchmark case.

Measured neutral (2.307 / 2.254 / 2.345 against 2.32), so the compiler was evidently already eliminating most of it. Kept because it states the intent rather than relying on that inference, and because a later change that makes those values live would otherwise reintroduce the cost silently.
Five copies of the fused L/R accumulation replaced by calls. IGR stores num_fluids - 1 volume fractions and derives the last as alpha(N) = 1 - sum, so by this point alpha is a full composition and the shared rule applies unchanged; for num_fluids == 1 it is exactly 1, which makes the bubbles special case and the general accumulation agree.

IGR carries no heat of formation, so the qv output is discarded into a local. That costs 16 B of scratch on igr_riemann_solver (252 -> 268), with registers unchanged.
H was computed as the last line of the routine and read by none of its three
callers. The non-IGR branch built E solely to feed it, so removing H also
removes that reconstruction and the local from two per-cell GPU loops, plus a
slot from each private() clause. The routine no longer computes an enthalpy, so
it is now s_compute_cell_state after what it does return.
A field that is analytically zero stores only roundoff seeded by the large
fields in the case, so comparing it pointwise tests the compiler's association
order: 421A6AD9 holds cons.9 with a maximum of 5.82e-07 against a case scale of
9.74e+08, one double epsilon, and a mathematically exact change flips the sign
of that crumb and fails by 17% of the absolute tolerance.

A field whose whole golden content lies within 1e-13 of the case's dominant
scale must now stay below that floor instead. The constant leaves 170x margin
on the observed noise, sits 7 orders below the relative tolerance used for real
comparisons, and classifies 2.4% of fields that have any nonzero content.

This also closes a hole: compute_error returns NaN when the golden value is
exactly 0 and is_close passes NaN unconditionally, so a field of exact zeros
was not checked at all. Roughly 2800 fields across the suite now have to stay
below the floor.
Both traps cost a CI cycle each on MFlowCode#1762 and neither is visible from a local amdflang or CCE OpenMP build. The CCE one also misreports its own location, so the note says to distrust the line number.
The base path formed the chemistry average state and then passed a literal zero for c_sum_Yi_Phi, and abs(c_c) > verysmall gates the branch, so it always fell back to the frozen sound speed while HLL and the hypoelastic variant used the Roe one. The same case got a different sound speed depending on the solver and on whether hypoelasticity was on.

The two are not interchangeable. c_c - (gamma - 1)*(vel_sum - H) reduces exactly to the Roe sound speed (gamma - 1)*(H_avg - |u_avg|^2/2) - the enthalpy and gamma*R*T terms cancel - while the fallback is the frozen mixture speed evaluated at pres_R with rho_avg and drops the velocity-variance term. They differ by about 2% on a representative shocktube state.

The bubbles loop keeps its literal zero and now says why: it never forms the chemistry average, and chemistry with bubbles_euler or qbmm is prohibited, so its branch is unreachable.

Of fourteen chemistry cases only the HLL one added here reached the Roe branch at all, so this combination had no coverage and no golden moves. Adds the HLLC case. Closes MFlowCode#1774.
Reattaches the s_compute_speed_of_sound docstring, which sat six lines above f_isentrope_exponent while its own routine, a hundred lines below, had none - Doxygen was pairing both with the wrong subprogram.

The rest is wording: the same facts in fewer lines, and three comments dropped for restating the statement under them. Comment lines only; no code changed.
A field counts as zero when every value lies within the absolute tolerance, which cannot separate roundoff from a real field whose magnitude equals that tolerance. Single precision scales QBMM's 1e-10 by 1e8, so its tolerance is 1e-2 and its initial bubble field is the constant 1e-2 exactly: the check read a physical field as zero, then failed the candidate for exceeding it by a few ulps. Eight QBMM cases failed that way on the single-precision lane and nowhere else.

Strictly inside, rather than a margin below: the crumb this check was written for, cons.9 in 421A6AD9, is 5.82e-07 against a 1e-6 tolerance, so any margin wide enough to exclude the constant field would reclassify the crumb and let the sign flip fail again. The two sit at 0.58x and 1.00x of the tolerance, and only the boundary separates them.

Also zeroes c_sum_Yi_Phi in HLLC's six-equation path. Chemistry is unreachable there, so the Roe branch never writes it, and passing it to the averaged sound speed read an undefined value - it had been a literal zero until the previous commit.
CCE faulted the GPU on the first step of both Roe-average chemistry cases: 'Memory access fault by GPU node-8 ... Reason: Unknown'. s_compute_chemistry_average_state called m_thermochem one routine deeper than master did, holding seven num_species automatic arrays in the intermediate frame. This is the rule the num_fluids arrays already follow - such calls belong in the parallel-loop body, not nested inside another device routine.

The enthalpies and heat capacities are now evaluated at the call sites and passed in, leaving the routine pure arithmetic over three arrays and no nested calls. Both call sites already computed Cp_iL and Cp_iR and did not modify them, so the routine had been recomputing them as well.

Also drops the chemistry branch from HLLC's six-equation loop, which model_eqns = 6eq cannot reach.

15 chemistry cases pass with no golden moving; chem-config GPU resources move by 2 registers on one kernel across 473, inside the noise that amdflang's link-time regeneration produces between identical builds.
Same shape as the num_fluids entry above it, but it builds clean and faults at runtime, so it is only visible from a case that reaches the path.
The previous commit made the classification strict, which fixed a field sitting exactly on the tolerance but not one sitting just under it. QBMM's bubble fields are constants at 0.9999997 of the tolerance in single precision, so they still classified as zero and still failed for drifting a few ulps past it - six cases, one timestep later than before.

The classification was never the problem; the band was. A zero field's candidate is now held to the larger of the tolerance and ten times the golden's own magnitude, so a field near the tolerance is judged against itself rather than against a number it happens to sit close to. A field storing genuine roundoff is orders below the tolerance, so its band is unchanged: the 5.82e-07 sign flip this check was written for still passes, and a candidate that grows from roundoff to 1e-3 still fails.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 244 out of 253 changed files in this pull request and generated 3 comments.

Comment on lines +102 to +106
# A field that is zero in the golden is checked for staying zero, not for
# reproducing its roundoff. This also closes a hole: a golden value of exactly 0
# gives a NaN relative error, which is_close() passes unconditionally, so such a
# field was previously not checked at all.
if _is_zero_field(gEntry.doubles, tol.absolute):
Comment on lines +25 to +26
mags = _magnitudes(values)
return bool(mags) and max(mags) <= atol
Comment on lines +1267 to +1269
call s_compute_mixture_coefficients(alpha_rho_K, alpha_K, rho, gamma, pi_inf, qv)

E = gamma*pres + pi_inf + 5.e-1_wp*rho*vel_sum + qv
Under bubbles_euler the last advection slot is the void fraction, not a material (eqn_idx%alf == eqn_idx%adv%end), so summing 1..num_fluids adds alpha_void*gammas(num_fluids) to the mixture. Unifying the solvers onto one rule that special-cased only num_fluids == 1 did exactly that, silently changing every two-fluid bubbly run - the case the PR description had said needed a ruling before any edit.

The published closure settles it. Bryngelson et al. write the liquid phase as Gamma_l*p_l = (E - rho|u|^2/2)/(1 - alpha) - Pi_inf_l: the liquid's own coefficients, undiluted, with the void entering only as the 1/(1 - alpha) on the energy. That is what s_compute_pressure and f_pressure already do, and what the old HLLC arm computed. The rule now reproduces that dispatch exactly for every bubbles_euler, num_fluids and mpp_lim combination, so HLLC and m_acoustic_src are unchanged and HLL and LF are corrected onto it.

Adds a 1D two-fluid bubbly case: nothing in the suite ran bubbles_euler above one fluid, which is why this was invisible. Also deletes eos_types, which was allocated, filled and never read.
The band check compared only the maximum, so a value leaving the band was reported without saying which, and the ten-fold headroom was wider than the rule needs. It is now applied per value, and the headroom is two - enough to cover a sign flip of a value already at the band edge and no more.

A rearrangement wholly inside the band stays invisible, and inherently so: values the test cannot resolve from zero cannot be resolved from each other. The docstring says that rather than implying the check is tighter than it is.
The ensemble-averaged closure is written for one carrier liquid: Gamma_l*p_l = (E - rho|u|^2/2)/(1 - alpha) - Pi_inf_l, the liquid's own coefficients undiluted, with the void occupying the last advection slot. Above two fluids the mixture rule has no derivation behind it and the existing arms disagree - one dilutes the liquid mixture by (1 - alpha), the other folds the void's own gamma in. Nothing in the suite ran it either way.

Rejected at validation rather than left to run an unvalidated closure. Closes MFlowCode#1786.
The pressure-based wave speed is Toro's stiffened-gas two-shock Mach number: (0.5 + Gamma)/(1 + Gamma) is (n + 1)/2n, and (p*/p - 1)*p divided by p only to multiply it back. The relativistic enthalpy was written out twice, once under a Hard-coded EOS comment; it carries no stiffness, so relativity with a nonzero pi_inf is now refused rather than silently given an ideal-gas answer.
@sbryngelson
sbryngelson force-pushed the fix/speed-of-sound-from-state branch from 2e6a1b8 to 8c973b5 Compare August 30, 2026 04:06
The bubble branch of the mixture sound speed is c = c_l/(1 - alf), the carrier-liquid speed with an O(alf) void correction - the same c_l m_qbmm forms for Keller-Miksis. alf is the subgrid void fraction and is small by construction, so a value near one is a wrong index or a case outside the expansion's regime, not a number to clamp. The toolchain now warns at case load. Closes MFlowCode#1793.
m_acoustic_src was the last site hand-rolling a sound speed. The Tait form it used is the same stiffened-gas expression as f_bulk_modulus, so that half is a pure de-duplication; the behaviour change is the subgrid dilution, which the hand-rolled code never applied. 39 bubbles_euler goldens move by ~1.7% at the source cell, which is 1/(1 - alf) at alf = 0.04 - visible because the test grids are coarse (49x39 in 2D). Inlined rather than calling s_compute_speed_of_sound, which faults CCE OpenACC from this loop (MFlowCode#1794).
@github-actions

Copy link
Copy Markdown

Lines of Code

File Lines Diff
src/common/m_variables_conversion.fpp 1124 +127
src/simulation/m_viscous.fpp 1021 -117
src/simulation/m_riemann_state.fpp 1191 +103
src/simulation/include/inline_riemann.fpp 0 -95
src/simulation/m_pressure_relaxation.fpp 152 -80
src/simulation/m_igr.fpp 2288 -47
src/simulation/m_riemann_solver_lf.fpp 517 -24
src/simulation/m_surface_tension.fpp 263 +23
src/simulation/m_bubbles_EL.fpp 1637 -22
src/simulation/m_acoustic_src.fpp 500 -21
src/simulation/include/inline_capillary.fpp 0 -17
src/simulation/m_riemann_solver_hll.fpp 595 +14
src/simulation/m_bubbles_EE.fpp 303 -13
src/simulation/m_riemann_solver_hypo_hlld.fpp 776 -13
src/simulation/m_riemann_solver_hlld.fpp 192 -11
src/simulation/m_cbc.fpp 1114 -10
src/post_process/m_data_output.fpp 1182 -8
src/common/m_phase_change.fpp 284 +7
src/simulation/m_data_output.fpp 1310 -7
src/simulation/m_ibm.fpp 1255 +6
src/simulation/m_riemann_solver_hllc.fpp 1268 -4
src/simulation/m_qbmm.fpp 878 -3
src/simulation/m_sim_helpers.fpp 193 -3
src/common/m_constants.fpp 87 +2
src/post_process/m_start_up.fpp 766 -2
src/simulation/m_rhs.fpp 1951 -2
src/common/m_derived_types.fpp 440 +1
src/common/m_global_parameters_common.fpp 239 +1
src/post_process/m_global_parameters.fpp 358 +1
src/post_process/p_main.fpp 54 -1
src/pre_process/m_global_parameters.fpp 429 +1
src/simulation/m_global_parameters.fpp 748 +1
src/simulation/m_hypoelastic.fpp 591 +1
src/simulation/m_reactive_burn.fpp 41 +1
src/simulation/m_start_up.fpp 1245 +1
src/simulation/m_time_steppers.fpp 827 -1
Directory Lines Diff
common 9916 +138
pre_process 4497 +1
simulation 27678 -340
post_process 3327 -10
total 45418 -211

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

bug: simulation probe sound speed omits qv from enthalpy, disagreeing with post-process

2 participants